module: fix addition of filetypes, if require ends with .. #7094
Conversation
|
Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion. Commit robertkowalski/node@b382ced has the following error(s):
The following commiters were not found in the CLA:
You can fix all these things without opening another issue. Please see CONTRIBUTING.md for more information |
|
You lied to me, Jenkins! Signed the CLA again (last commit is almost a year old) |
|
Jenkins is broken. Don't worry about it. :-) |
|
it's been a few months, any chance that this gets into a release? |
|
@tjfontaine Does this functionality better match what you'd expect? |
|
I spoke some with @robertkowalski in the irc channel, there are a lot of things in the module loader that behave implicitly, for instance relying on the ordering of property iteration from v8 as opposed to explicitly defining the preferred ordering. It's not always easy to fix things in the module loader without also breaking the legacy tail. In this particular case the intention is to It's important that how this works also falls in line with how the work we want to do for bundles is done, so I'm going to /cc @bmeck |
|
The logic should not affect bundles. Bundles are treated as files that when encountered act like directories. Though, I do think the extra stat calls are going to hit even when we do use bundles since you need to associate directories w/ which kind of loader the directory has (bundled vs. fs). |
|
Ok, had some offline conversation with TJ in Lisbon. Here is what I did: I updated the PR to use Created some benchmarks, which can be found at: https://github.com/robertkowalski/require-benchmark - I first created the data, then rebooted and started testing. After each run I rebooted again. After each reboot I waited 20 sec. I think it is important to note that The first version of my benchmark had a little error that led to interesting end results: If a package with So we'll see that the modified version, which tests for directories is slightly faster in the synthetic testcases As mentioned, it gets slower (around 1000s on the first call) if the Results require-benchmark@latest (0cc5fde0ee83524bea97d4bd4f6296f981aa4a99) :hapi - modifiedhapi - current masterflat - modifiedflat - current masternested - modifiednested - current masterold benchmark versions (the ones that use tryExtensions as they miss a package)flat - modified - old version of benchmarkflat - current master - old version of benchmarknested - modified - old version of benchmarknested - current master - old version of benchmark |
robertkowalski
force-pushed
the
fix/require-slice
branch
2 times, most recently
from
f0580d5
to
72f26cc
Compare
|
@tjfontaine let's talk during this nodeconf.eu about that |
Given my home-directory is `/Users/rocko` - and I have a file named
`npm.json` in it and also a repository with name `npm`, which is a
folder for the node-module.
When try to require the `/Users/rocko/npm/index.js` two direcotry
levels down in the npm folder (e.g. `/Users/rocko/npm/test/tap`)
with require("../../") node will load `/Users/rocko/npm/index.json`.
When I use require("../..") node will load `/Users/rocko/npm.json`
which is fixed by this commit.
robertkowalski
force-pushed
the
fix/require-slice
branch
from
72f26cc
to
a294d5d
Compare
|
Using the new included benchmark I get the following results: master 0.12 patch |
|
@robertkowalski ... still need/want this? |
|
it landed in io.js along with other improvements to the module loader On Mon, Aug 3, 2015 at 11:10 PM, James M Snell notifications@github.com
|
|
Do you see a reason to land it in v0.12? Or can we close this and pick it up under the converged stream? |
|
it is a bugfix, but a breaking fix. the idea was to land it in 0.11 and get it into the 0.12 release. i am happy if it lands in the next versions, so we can close it. :) |
Given my home-directory is
/Users/rocko- and I have a file namednpm.jsonin it and also a repository with namenpm, which is anode-module.
When try to require the
/Users/rocko/npm/index.jstwo direcotrylevels down in the npm folder (e.g.
/Users/rocko/npm/test/tap)with
require("../../")node will load/Users/rocko/npm/index.js.When I use
require("../..")node will load/Users/rocko/npm.jsonwhich is fixed by this commit.